Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[WIP] Add Ratelimiting test using Envoy's Ratelimit Service #23513

Closed
wants to merge 4 commits into from
Closed

[WIP] Add Ratelimiting test using Envoy's Ratelimit Service #23513

wants to merge 4 commits into from

Conversation

gargnupur
Copy link
Contributor

@gargnupur gargnupur commented May 4, 2020 •
edited by istio-policy-bot

This PR adds an integration test that starts envoy's open source ratelimit service : https://github.com/envoyproxy/ratelimit and use envoy's ratelimit filter(https://www.envoyproxy.io/docs/envoy/latest/configuration/http/http_filters/rate_limit_filter) to actually ratelimit calls from Istio.

Ref: #22068

@gargnupur
ratelimit test
a5d116f 
add root

get policy

add header
@gargnupur
Add Ratelimit test using Envoy Ratelimit Service
7ca89d0 
Signed-off-by: gargnupur <gargnupur@google.com>
@gargnupur
Fix lint
654c2ac
@istio-testing istio-testing added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label May 4, 2020
@googlebot googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label May 4, 2020
@istio-testing istio-testing added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 4, 2020
@gargnupur
Copy link
Contributor Author

gargnupur commented May 4, 2020 •
edited

@mandarjog , @bianpengyuan , @douglas-reid , @howardjohn , @nmittler : Have a few questions for the integration test:

  1. Test uses bookinfo application as we would like to test ratelimiting between different services in mesh later.. should I still use echo application in integration test?
  2. The docker file to configure ratelimit service is currently hosted on my project's registry. Would need help uploading it to istio-testing... (Couldn't use publicly hosted dockerfile as ratelimit service uses a config file to configure ratelimit rules and that is copied inside dockerfile)

@gargnupur
fix lint error
f49c2d6 
Signed-off-by: gargnupur <gargnupur@google.com>
@nmittler
Copy link
Contributor

nmittler commented May 4, 2020

Test uses bookinfo application as we would like to test ratelimiting between different services in mesh later.. should I still use echo application in integration test?

Is there an issue with using echo for rate limiting? Not sure if it's easier with bookinfo for some reason and why.

howardjohn
howardjohn reviewed May 4, 2020
View reviewed changes
Copy link
Member

@howardjohn howardjohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 on Nates comment, not sure why we cannot use echo? you can have multiple Services.

Overall I am a little uncertain if this is the appropriate level to test this out. As far as I can tell this is essentially testing an Envoy feature - we don't even have an API to expose the Envoy ratelimiting. I would think this would just be a unit test to ensure that EnvoyFitler works as expected.
I do see there is some value as adding it as a tested example, but there cost here is fairly large (maintaining some new ratelimit service, etc). If we are going to invest this much effort, should we define a proper API for this?

tests/integration/policy/ratelimiting_test.go
// See the License for the specific language governing permissions and
// limitations under the License.

package policy
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you make a new package it won't run in CI without also adding a new job

@mandarjog
Copy link
Contributor

@gargnupur these tests should be in some other place...

@nmittler

  1. We want to add tests that are a companion of tasks that we are going to add to istio.io/docs.
    Do you recommend and alternate place?
  2. Given (1) we should use the app that makes sense on the doc site. @rcaballeromx is working on using a single example. hipstershop, bookinfo. Echo is useful for unit / integration tests not user facing tests.
  3. (1) is being done due to user demand, we have a way to do rate limiting in Mixer V1. And our position for v2 so far was "hey user, you figure it out. its just Envoy".
  4. I am planning to add a rate limiting extensions API, but we have been very shy adding any APIs, so we must have a doc to point people to.

@gargnupur
Copy link
Contributor Author

@mandarjog , @nmittler , @howardjohn : Thanks for the reply! Yes, this Draft PR was to get us to start talking and have a working example of how users can do rate limiting in Istio without Mixer...

@nmittler
Copy link
Contributor

nmittler commented May 5, 2020

@mandarjog

  1. We want to add tests that are a companion of tasks that we are going to add to istio.io/docs.
    Do you recommend and alternate place?

By "companion" do you mean that these are the tests for the documentation? If so, they belong here: https://github.com/istio/istio.io/tree/master/tests

  1. Given (1) we should use the app that makes sense on the doc site. @rcaballeromx is working on using a single example. hipstershop, bookinfo. Echo is useful for unit / integration tests not user facing tests.

If these are docs tests, then yes ... you have to do whatever is documented.

@gargnupur gargnupur mentioned this pull request May 13, 2020
Closed
@gargnupur gargnupur mentioned this pull request Jun 1, 2020
Closed
@istio-policy-bot istio-policy-bot added the lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while label Jun 5, 2020
@gargnupur gargnupur mentioned this pull request Jun 10, 2020
Open
@douglas-reid
Copy link
Contributor

@gargnupur is this still important to get in? It feels like it, but I'm not clear on the status here.

@istio-policy-bot istio-policy-bot removed the lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while label Jun 13, 2020
@gargnupur
Copy link
Contributor Author

@gargnupur is this still important to get in? It feels like it, but I'm not clear on the status here.

@douglas-reid : yes it is.. it just keeps going to back of my queue! I need to move this to istio.io.. will try to do it soon...

@istio-testing
Copy link
Collaborator

@gargnupur: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
analyze-tests_istio f49c2d6 link /test analyze-tests_istio

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@istio-policy-bot istio-policy-bot added the lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while label Jul 13, 2020
@istio-policy-bot
Copy link

🚧 This issue or pull request has been closed due to not having had activity from an Istio team member since 2020-06-13. If you feel this issue or pull request deserves attention, please reopen the issue. Please see this wiki page for more information. Thank you for your contributions.

Created by the issue and PR lifecycle manager.

@istio-policy-bot istio-policy-bot added the lifecycle/automatically-closed Indicates a PR or issue that has been closed automatically. label Jul 28, 2020
@gargnupur gargnupur mentioned this pull request Sep 10, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@howardjohn howardjohn howardjohn left review comments

@mandarjog mandarjog Awaiting requested review from mandarjog

@nmittler nmittler Awaiting requested review from nmittler

@bianpengyuan bianpengyuan Awaiting requested review from bianpengyuan

@douglas-reid douglas-reid Awaiting requested review from douglas-reid

Assignees
No one assigned
Labels
area/extensions and telemetry cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. lifecycle/automatically-closed Indicates a PR or issue that has been closed automatically. lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@gargnupur @nmittler @mandarjog @douglas-reid @istio-testing @istio-policy-bot @howardjohn @googlebot